remove buggy useEffect usage #1316
Conversation
miles-grant-ibigroup
assigned daniel-heppner-ibigroup and unassigned miles-grant-ibigroup
daniel-heppner-ibigroup
assigned binh-dam-ibigroup and unassigned daniel-heppner-ibigroup
|
|
||
| // Update state when external state is updated | ||
| useEffect(() => { | ||
| if (initialDate !== date) setDate(initialDate) | ||
| if (initialTime !== time) { | ||
| setTime(initialTime) | ||
| handleTimeChange(initialTime || '') | ||
| } | ||
| }, [initialTime, initialDate]) |
There was a problem hiding this comment.
eslint is highlighting missing dependencies here: date, time, and handleTimeChange, could you add them to the dependency array as well.
There was a problem hiding this comment.
the problem is these useEffect hooks are propagating updates from the store into the component, adding those deps would cause a loop. Otherwise I would have just replaced them with callbacks
There was a problem hiding this comment.
@daniel-heppner-ibigroup Okay, could you add a comment about the undesired side effect?
There was a problem hiding this comment.
There is one in caf809c
| ) | ||
| } | ||
| }, [time]) | ||
|
|
||
| useEffect(() => { | ||
| if (initialDepartArrive && departArrive !== initialDepartArrive) { | ||
| setDepartArrive(initialDepartArrive) | ||
| } | ||
| }, [initialDepartArrive]) |
There was a problem hiding this comment.
While we are in fixing dependency arrays, could you adding departArrive in here too per eslint.
There was a problem hiding this comment.
same as above, causes a loop
| }, [departArrive, setTime, setDate, homeTimezone]) | ||
| } | ||
| }, | ||
| [syncSortWithDepartArrive, sort, importedUpdateItineraryFilter] |
There was a problem hiding this comment.
Here, eslint is suggesting to add dateTime, handleTimeChange, homeTimezone to the dependency array.
There was a problem hiding this comment.
Good note, added them.
daniel-heppner-ibigroup
assigned binh-dam-ibigroup and unassigned daniel-heppner-ibigroup
Description:
These useEffect usages were causing bugs. Since useEffect calls its callback any time one of the dependencies change, it can be easy to accidentally introduce unexpected bugs. In this case, the query params changing was causing the dateTime effect to be called, which was setting the itinerary sort when external parameters were changing. In this case, we noticed changing the sort order from the itinerary header was causing the effect to run, immediately overriding the setting.
This is actually a good lesson in why useEffect is dangerous!